-
Notifications
You must be signed in to change notification settings - Fork 319
Fix Quick Match cancel button not responding #673
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Conversation
|
π Welcome @Rozerxshashank!
We appreciate your contribution! π |
π WalkthroughWalkthroughThe Changes
Estimated code review effortπ― 3 (Moderate) | β±οΈ ~20 minutes Poem
π₯ Pre-merge checks | β 5β Passed checks (5 passed)
βοΈ Tip: You can configure your own custom pre-merge checks in the settings. π Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro π Files selected for processing (1)
π§ Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Hi @M4dhav π |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
π§Ή Nitpick comments (1)
lib/controllers/pair_chat_controller.dart (1)
241-260: Excellent fix for the race condition!The try/catch/finally structure with null-guarded deletion properly addresses the bug described in issue #670. The finally block ensures navigation always occurs, preventing the stuck UI, and the null check prevents the crash when cancel is tapped before the document is created.
Optional: Move
requestDocId = nullto the finally block for guaranteed cleanup.Currently, if document deletion fails (lines 244-248), the exception is caught but
requestDocIdremains non-null until navigation completes. WhileGet.offNamedUntilshould dispose the controller, moving the reset tofinallywould be more defensive:π View suggested refactor
Future<void> cancelRequest() async { try { if (requestDocId != null) { await databases.deleteDocument( databaseId: masterDatabaseId, collectionId: pairRequestCollectionId, documentId: requestDocId!, ); } - - requestDocId = null; - await subscription?.close(); await userAddedSubscription?.close(); } catch (e) { log('Cancel request failed: $e'); } finally { + requestDocId = null; Get.offNamedUntil(AppRoutes.tabview, (route) => false); } }Optional: Consider independent error handling for subscriptions.
If
subscription?.close()throws on line 253,userAddedSubscription?.close()won't execute. Wrapping each in separate try/catch would ensure both closures are attempted:π View alternative approach
Future<void> cancelRequest() async { try { if (requestDocId != null) { await databases.deleteDocument( databaseId: masterDatabaseId, collectionId: pairRequestCollectionId, documentId: requestDocId!, ); } } catch (e) { log('Failed to delete request document: $e'); } try { await subscription?.close(); } catch (e) { log('Failed to close subscription: $e'); } try { await userAddedSubscription?.close(); } catch (e) { log('Failed to close userAddedSubscription: $e'); } requestDocId = null; Get.offNamedUntil(AppRoutes.tabview, (route) => false); }This provides more granular error reporting and guarantees all cleanup attempts execute.
|
Please fix merge conflicts |
|
For future contributions, please strictly follow the PR Template |
|
Resolved the merge conflict and restored the safe cancelRequest implementation. |
|
Hi @M4dhav π |
|
Hey @Rozerxshashank , automated tests are failing |
M4dhav
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix tests failure
|
Hey @M4dhav, Iβve pushed the updated fix for PairChatController.cancelRequest. |
| ); | ||
| } | ||
|
|
||
| requestDocId = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why set the requestDocId to null?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I set to null beacuse if app keeps the old requestDocId it may try to cancel the same request again. (Like when button tapped twice)
It just avoids accidental reuse of an invalid ID and reset the controlller to a complete clean state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great idea!
M4dhav
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, thank you for the contribution.
Could you share a video of the functionality with this fix please?
|
@coderabbitai review |
β Actions performedReview triggered.
|
π Bug
The Cancel button in Quick Match could be tapped before the pairing request
was fully created, leading to a null-assertion crash and causing the UI
to get stuck on the loading screen.
β Fix
requestDocIdfinallyblockπ§ͺ Testing
Fixes #670
Summary by CodeRabbit
βοΈ Tip: You can customize this high-level summary in your review settings.